Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do Not Invalidate Current Session When Its Registered #15066

Closed
wants to merge 2 commits into from

Conversation

joaquinjsb
Copy link
Contributor

@joaquinjsb joaquinjsb commented May 13, 2024

this change prevents from deleting the current session, which ends up creating the need to log in two times.

this change prevents from deleting the current session to, creating the need to log in two times.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 13, 2024
@marcusdacoregio
Copy link
Contributor

Hi @joaquinjsb, can you elaborate more on what is the problem and how this fixes it? As far as I can tell the invalidation is performed before the current session is registered in the ReactiveSessionRegistry, thus not returning the current session.

@marcusdacoregio marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2024
@marcusdacoregio marcusdacoregio self-assigned this May 13, 2024
@joaquinjsb
Copy link
Contributor Author

joaquinjsb commented May 13, 2024

Hi @joaquinjsb, can you elaborate more on what is the problem and how this fixes it? As far as I can tell the invalidation is performed before the current session is registered in the ReactiveSessionRegistry, thus not returning the current session.

what happens in my case is:
-> login browser 1
-> login browser 2
-> the ConcurrentSessionControlServerAuthenticationSuccessHandler:handleConcurrency finds currentSession, and the
registeredSessions = sessionTuple.getT2(); contains both the current session and the session from browser 1 and passes it down on line 87.

what my fix does is, removes from the registeredSessions, the current session, so that you don't end up deleting both browser 1 and browser 2 sessions, which prevents the login flow from being "standard" on the user side.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 13, 2024
@marcusdacoregio
Copy link
Contributor

There is a test that verifies that behavior here, can you create a new test that reproduces the problem?

@joaquinjsb
Copy link
Contributor Author

I tried running it with a limit of 1, which behaves correctly as you describe, would the problem be probably on SpringSessionBackedReactiveSessionRegistry? do you have any suggestions on how to proceed?

@marcusdacoregio
Copy link
Contributor

The best approach would be to create a minimal, reproducible sample for the issues, with tests preferably. Then we can identify exactly where the problem is

@joaquinjsb
Copy link
Contributor Author

The best approach would be to create a minimal, reproducible sample for the issues, with tests preferably. Then we can identify exactly where the problem is

here you have both minimal reproducible, and a test which is failing.
https://github.com/joaquinjsb/sessionRedisReactive

@joaquinjsb
Copy link
Contributor Author

joaquinjsb commented May 13, 2024

I dived in further, the answer to our question is here:
https://github.com/spring-projects/spring-security/blob/a17a599bd54838dd2f166d97555de3240335d07f/config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java#L2120C5-L2127C6

the delegate.getSession() creates a new session, then it gets saved in the repository, which means that the getSessions() will have both the currentSession and the older ones.

so why it doesn't reflect in the tests is due to the fact that both InMemoryReactiveSessionRegistry & WebSessionStore has their own sessionsMaps.

I can apply my change in the handler, so filters don't need to do that work, or as I did, or do you recommend another path?

@marcusdacoregio marcusdacoregio changed the title Update InvalidateLeastUsedServerMaximumSessionsExceededHandler.java Do Not Invalidate Current Session When Its Registered May 14, 2024
@marcusdacoregio marcusdacoregio added this to the 6.3.0 milestone May 14, 2024
@marcusdacoregio
Copy link
Contributor

Thanks @joaquinjsb, this has been closed via 927840f

@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) type: bug A general bug and removed status: feedback-provided Feedback has been provided labels May 14, 2024
@joaquinjsb joaquinjsb deleted the patch-1 branch May 14, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants